-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor: Simplify transaction handling and gas calculations #2262
Conversation
- Removed redundant transaction type checks in MaybeCheckedTransaction::id() by centralizing the logic. - Refactored max_gas() method to handle common transaction types (Script, Create, Upgrade, Upload, and Blob) in a single match arm, reducing code duplication. - Preserved special handling for Mint transactions, which do not have max_gas. - Improved overall code clarity and maintainability.
Moved transaction JSON deserialization into a separate function to reduce code duplication and improve code clarity across all transaction-related commands. This refactor makes the code easier to maintain and enhances readability by centralizing the JSON parsing logic. It affects all commands that process transactions, such as Submit, EstimatePredicates, DryRun, and others.
This update refactors the test suite by extracting the common logic for creating tests into a helper function create_test. This helps reduce code duplication and improves code readability and maintainability across the test suite.
Could you try to run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert formatting changes, it is too many modifications
There seem to be a lot of formatting and other changes that weren't there when I first reviewed. |
Hi! After running the check and fixing the issues in my commits, all problems have been resolved. Please take a look at the changes and let me know if anything else needs attention. Thanks for your patience! |
The script |
I am closing this PR for now since the comment to revert formatting changes has not been resolved for a long period of time. If you still want to contribute, please address comments and create a new PR with link to this one=) |
Linked Issues/PRs
N/A
Description
This refactor simplifies transaction handling and gas calculations by:
MaybeCheckedTransaction::id()
by centralizing the logic.max_gas()
method to handle common transaction types (Script, Create, Upgrade, Upload, and Blob) in a single match arm, reducing code duplication.max_gas
.Checklist
Before requesting review
After merging, notify other teams
[Add or remove entries as needed]